fix(ci): update .main.commit for non-dev mcore branches - #3214
Conversation
When bumping on release branches (e.g. r0.4.0 with mcore-core_r0.17.0), the mcore-branch input matches neither "main" nor "dev", so .main.commit was never written. The unit test test_main_commit_matches_submodule then fails because the submodule no longer matches .main.commit. Fix: write .main.commit for all non-dev branches (main + release branches). Signed-off-by: oliver könig <okoenig@nvidia.com>
📝 WalkthroughWalkthroughModified a GitHub Actions workflow step to change how commit marker files are written based on branch. Now writes Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/_update_dependencies.yml (1)
74-76: Consider renaming the localTARGET_BRANCHto avoid shadowing.The
TARGET_BRANCHenv var here refers toinputs.mcore-branch, but there's already aTARGET_BRANCHdefined at line 43 referring toinputs.target-branch. While shell scoping keeps these separate per step, the reuse is potentially confusing for future maintainers.♻️ Suggested rename for clarity
- name: Update submodules id: mlm-commit env: - TARGET_BRANCH: ${{ inputs.mcore-branch }} + MCORE_BRANCH: ${{ inputs.mcore-branch }} run: | pushd 3rdparty/Megatron-LM git fetch origin ${{ inputs.mcore-branch }} git checkout FETCH_HEAD MLM_COMMIT=$(git rev-parse HEAD) popd - if [[ "$TARGET_BRANCH" == "dev" ]]; then + if [[ "$MCORE_BRANCH" == "dev" ]]; then echo $MLM_COMMIT > .dev.commit else echo $MLM_COMMIT > .main.commit fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/_update_dependencies.yml around lines 74 - 76, The step defines env TARGET_BRANCH = inputs.mcore-branch which shadows the earlier TARGET_BRANCH = inputs.target-branch; rename the local variable (e.g., MCORE_TARGET_BRANCH or MC_TARGET_BRANCH) in this step's env and update any downstream uses in that step to reference the new name so it's clear this is the mcore branch and not the global TARGET_BRANCH; ensure you change occurrences of TARGET_BRANCH inside this step (and any subsequent step-scoped references) to the new symbol while leaving the original inputs.target-branch binding untouched.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.github/workflows/_update_dependencies.yml:
- Around line 74-76: The step defines env TARGET_BRANCH = inputs.mcore-branch
which shadows the earlier TARGET_BRANCH = inputs.target-branch; rename the local
variable (e.g., MCORE_TARGET_BRANCH or MC_TARGET_BRANCH) in this step's env and
update any downstream uses in that step to reference the new name so it's clear
this is the mcore branch and not the global TARGET_BRANCH; ensure you change
occurrences of TARGET_BRANCH inside this step (and any subsequent step-scoped
references) to the new symbol while leaving the original inputs.target-branch
binding untouched.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6a2bf504-1a82-448b-ae76-4f72e5416f63
📒 Files selected for processing (1)
.github/workflows/_update_dependencies.yml
Summary
Backport of #3213 to
r0.4.0.r0.4.0withmcore-branch=mcore-core_r0.17.0), themcore-branchinput matches neither"main"nor"dev", so.main.commitwas never writtentest_main_commit_matches_submodulethen fails because the bumped submodule commit no longer matches.main.commit— as seen in chore(beep boop 🤖): Bumpuv.lock(r0.4.0, mcore-core_r0.17.0) (2026-04-07) #3191.main.commitfor all non-devbranches (covers bothmainand release branches)Test plan
test_main_commit_matches_submodulepasses on next bump run forr0.4.0Summary by CodeRabbit